[PM-24978] Pass along Attachment decryption errors #644
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
🔍 SDK Breaking Change Detection ResultsSDK Version:
Breaking change detection completed. View SDK workflow |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #644 +/- ##
==========================================
+ Coverage 79.35% 79.43% +0.07%
==========================================
Files 293 293
Lines 32264 32387 +123
==========================================
+ Hits 25603 25726 +123
Misses 6661 6661 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| let (file_name, decryption_failure) = match self.file_name.decrypt(ctx, key) { | ||
| Ok(name) => (name, false), | ||
| Err(..) => (None, true), | ||
| }; |
There was a problem hiding this comment.
Currently the SDK swallows any decryption errors for Attachments and thus hides all attachments from the user.
Where is the actual error being swallowed? I would suggest adding the handling logic there, maybe following the pattern of decrypt_list_with_failures. with the change here, you end up throwing away the CryptoError and the function returns as a "successful" decryption, breaking the contract of "This function returns a decrypted object, or an Err(CryptoError)"
There was a problem hiding this comment.
The error is being swallowed when the cipher list is being decrypted here.
maybe following the pattern of decrypt_list_with_failures
This could work but it would expand the API of the Cipher. I could leave the existing attachments alone and only return the decrypted attachments and then add a attachment_failure property. What do you think about that?
There was a problem hiding this comment.
Added my approach here: 3d5dfac
I appreciate the feedback, let me know what you think!
… failure attachments
| /// Attachments that failed to decrypt. Only present when there are decryption failures. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub attachment_decryption_failures: Option<Vec<attachment::AttachmentView>>, |
There was a problem hiding this comment.
nit: Maybe move this to just below line 347?
| attachments: &[Attachment], | ||
| ctx: &mut KeyStoreContext<KeyIds>, | ||
| key: SymmetricKeyId, | ||
| ) -> (Option<Vec<AttachmentView>>, Option<Vec<AttachmentView>>) { |
There was a problem hiding this comment.
(Vec<AttachmentView>, Vec<AttachmentView>) might be a better choice here (foregoing wrapping them in Option) - an empty Vec conveys the same information and simplifies a lot of the downstream logic.
There was a problem hiding this comment.
I do like removing the Option as it cuts down on the needed code and mental thought. It doesn't match the definitions on the CipherView so I had to rewrap in Some but that feels like an okay trade off
| ciphers_key, | ||
| ) | ||
| }) | ||
| .unwrap_or((None, None)); |
There was a problem hiding this comment.
optional nit: .unwrap_or_default() has the same effect here.
🎟️ Tracking
PM-24978
Clients PR: bitwarden/clients#17790
📔 Objective
Currently the SDK swallows any decryption errors for Attachments and thus hides all attachments from the user. Even when the user has multiple attachments on a cipher.
This adds a new
attachment_decryption_failuresattribute toCipherViewthat is populated with all attachments that fail to decrypt. This can then be consumed by clients to allow the user to delete any corrupted ciphers.🚨 Breaking Changes
❓ This could be a breaking change for the mobile clients if they expect attachment decryptions to be ignored. I do not know if that is the case yet. I'll reach out.No additional changes only.
Client UI
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes